chore(dashboards): Discover split for self hosted dashboard discover widgets #92135
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #92135 +/- ##
==========================================
- Coverage 87.93% 87.90% -0.04%
==========================================
Files 10174 10193 +19
Lines 583385 583854 +469
Branches 22596 22606 +10
==========================================
+ Hits 512992 513218 +226
- Misses 69941 70184 +243
Partials 452 452 |
|
This PR has a migration; here is the generated SQL for for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
| ) -> None: | ||
| DashboardWidgetQuery = apps.get_model("sentry", "DashboardWidgetQuery") | ||
| catch_all_unsplit_widgets = Q( | ||
| widget__widget_type=DashboardWidgetTypes.DISCOVER, |
There was a problem hiding this comment.
Let's copy over all the enums into the migration file- it's a good practice to not reference application code in the migration. In case someone changes the enums in the application code, we know it's not going to break the migration if it's copied over here
| queryset = DashboardWidgetQuery.objects.filter( | ||
| widget__widget_type=DashboardWidgetTypes.ERROR_EVENTS, | ||
| widget__dataset_source=DatasetSourcesTypes.UNKNOWN.value, | ||
| ).select_related("widget__dashboard__organization") |
There was a problem hiding this comment.
Don't need the select related here. Also, this revert is missing transaction widgets:
queryset = DashboardWidgetQuery.objects.filter(
widget__discover_widget_split__in=[DashboardWidgetTypes.ERROR_EVENTS, DashboardWidgetTypes.TRANSACTION_LIKE],
)
| try: | ||
| _get_and_save_split_decision_for_dashboard_widget(widget_query, dry_run=False) | ||
| except Exception: | ||
| widget_query.widget.widget_type = DashboardWidgetTypes.ERROR_EVENTS |
There was a problem hiding this comment.
_get_and_save_split_decision_for_dashboard_widget only updates discover_widget_split and dataset_source, so that's what we should do here also. Let's not update widget_type here.
|
This PR has a migration; here is the generated SQL for for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
| """ | ||
| TRANSACTION_LIKE = 101 | ||
| """ | ||
| This targets transaction-like data from the split from discover. Itt may either use 'Transactions' events or 'PerformanceMetrics' depending on on-demand, MEP metrics, etc. |
There was a problem hiding this comment.
| This targets transaction-like data from the split from discover. Itt may either use 'Transactions' events or 'PerformanceMetrics' depending on on-demand, MEP metrics, etc. | |
| This targets transaction-like data from the split from discover. It may either use 'Transactions' events or 'PerformanceMetrics' depending on on-demand, MEP metrics, etc. |
| """ | ||
| SPANS = 102 | ||
| """ | ||
| These represent the logs trace item type on the EAP dataset. |
There was a problem hiding this comment.
Is this supposed to be for logs or spans?
There was a problem hiding this comment.
This description is supposed to be for logs. I'll move it to after the logs definition for consistency 👍
| from django.db.migrations.state import StateApps | ||
| from django.db.models import Q | ||
|
|
||
| from sentry.discover.dashboard_widget_split import _get_and_save_split_decision_for_dashboard_widget |
There was a problem hiding this comment.
We generally don't import from application code into migrations, as import errors can creep in. I remember this came up in a previous discover dataset split discussion, and there was enough logic in this that cloning it into the migrations wasn't simple. I'm assuming that is still the case, and that we're going to be careful with this function for as long as this migration lives (which could be years).
There was a problem hiding this comment.
yup same case, we'll be keeping an eye on this function
The discover split for dashboards has been done for all users except self-hosted users. This migration converts all
Discoverwidget types to eitherTransactionorErrorusing the_get_and_save_split_decision_for_dashboard_widgetfunction. This function will try to appropriately categorize a widget query asTransactionorErrorand will default toErrorif a decision cannot be made. This uses the same logic of the original discover split job.